Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use monotonic time #160

Merged
merged 1 commit into from Feb 9, 2023
Merged

use monotonic time #160

merged 1 commit into from Feb 9, 2023

Conversation

mikz
Copy link
Contributor

@mikz mikz commented May 10, 2016

see ruby-concurrency/concurrent-ruby#256 for more information

Main reason is that your test can stub Time. So measuring duration of tests by that can get affected.
Similar thing was resolved in cucumber 2. See cucumber/cucumber-ruby-core#101

@shepmaster
Copy link
Member

Thanks! I don't suppose there is a gem that would provide this logic already, is there? It seems pretty out-of-scope for CI::Reporter to know about the underlying Ruby interpreter!

Out of curiosity, did you experience any concrete problems from the use of a time source that could be monkeyed with?

@shepmaster
Copy link
Member

a gem that would provide this logic

Especially if the same thing has already been copied into two other projects...

@mikz
Copy link
Contributor Author

mikz commented May 10, 2016

@shepmaster there is concurrent-ruby implementation, but that looks like an overkill. I have not seen any other reasonable cross platform implementation.

Yes, my test suite runs -30 days and other random numbers.

@mikz
Copy link
Contributor Author

mikz commented May 10, 2016

Just btw, I as a developer would pick copied code like this in my dependencies over having possibility of a version clash when trying to upgrade. Or installing another gem.

@mikz
Copy link
Contributor Author

mikz commented Jun 2, 2016

@shepmaster this is rebased and green. I really don't think it is worth to abstract this somewhere.
You generally needs this in concurrent environments for timeouts or when measuring duration of tests (which can stub time).

@shepmaster
Copy link
Member

I really don't think it is worth to abstract this somewhere.

I understand your feelings, but I don't agree with them 😸 As you can tell by my long response times, this is a low-touch project, and I'm not excited to maintain ~30 lines of code that from all appearances appear to be highly specific to a given Ruby interpreter.

I do agree that this kind of thing would be highly valuable. I could see accepting a PR that refactors CI::Reporter to allow injecting some kind of TimeSource. That would allow you to maintain those 30 lines of code in each of your projects, while not having to maintain a hacked-up version of CI::Reporter. How's that sound?

@mikz
Copy link
Contributor Author

mikz commented Jun 9, 2016

How's that sound?

Better than having a gem dependency.

But still I don't think it is acceptable default experience. Have you considered adding more maintainers to the project, so it is not just yours responsibility?

@Bertg
Copy link

Bertg commented Apr 25, 2018

Process.clock_gettime(Process::CLOCK_MONOTONIC) is supported by jRuby >9.0.0. So we could remove the check for jRuby. Older jRuby version would still use the Time.now fallback.

As a compromise between the argument It seems pretty out-of-scope for CI::Reporter to know about the underlying Ruby interpreter and the desire to add monotonic time.

Would that be acceptable?

@mikz
Copy link
Contributor Author

mikz commented Apr 25, 2018

@Bertg great! Will update the patch. Looks like there is no Process::CLOCK_MONOTONIC in JRuby 1.7.

@mikz
Copy link
Contributor Author

mikz commented Apr 25, 2018

@shepmaster is it small enough now? It is really just getting time.

@Bertg
Copy link

Bertg commented Apr 25, 2018

Looks great to me 👍

@Bertg
Copy link

Bertg commented Apr 25, 2018

@shepmaster I was actually planning to make this PR as well. I got inspired by this post: https://blog.dnsimple.com/2018/03/elapsed-time-with-ruby-the-right-way/

@nicksieger nicksieger merged commit 3590630 into ci-reporter:main Feb 9, 2023
@nicksieger
Copy link
Member

Looks ok to me, let's try this.

@mikz mikz deleted the monotonic-time branch February 9, 2023 18:13
@Bertg
Copy link

Bertg commented Feb 10, 2023

I have never seen a PR this old get merged in. Without any sense of sarcasm: this is amazing.
Thanks @nicksieger for picking this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants